Skip to content

fix(models): strip ANSI, not just Rich markup, from download-server error text (BE-5023) - #627

Open
mattmillerai wants to merge 7 commits into
mainfrom
matt/be-5023-sanitize-download-errors
Open

fix(models): strip ANSI, not just Rich markup, from download-server error text (BE-5023)#627
mattmillerai wants to merge 7 commits into
mainfrom
matt/be-5023-sanitize-download-errors

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

STACKED — merging lands on matt/be-4794-sanitize-ansi (owned by @mattmillerai, PR #614), NOT main. This PR is based on #614's branch because it uses comfy_cli/output/sanitize.py and the pretty / assert_inert() test harness, neither of which exists on main yet. Review the two-commit diff, or wait for #614 to merge and GitHub will retarget this to main automatically. Do not merge this before #614.

ELI-5

When a model download fails, the CLI prints the reason the server gave us. A hostile (or impersonated) model host gets to pick that text — and it can hide invisible terminal control codes in it that clear your screen, rename your terminal window, or scribble over lines the CLI already printed to make them say something else. We were already defusing one half of that (Rich markup), but not the escape codes themselves. Now we strip both, at the point the server's text enters the CLI and again where it is printed.

What was wrong

comfy_cli/command/models/models.py rendered download-server error text with rich.markup.escape(). The inline comment said what it was for: stopping a MarkupError from a stray [/]. It does that, but escape() does not touch raw ANSI/control bytes, so server-chosen escape sequences still reached the terminal live.

The remote text arrives via guess_status_code_reason() in comfy_cli/file_utils.py. Most branches return a canned per-status string and are safe; the 401 branch interpolates the server's own JSON message. That flows into DownloadException and out through the two print sites. Reproduced on origin/main:

body = json.dumps({"message": "\x1b[2J\x1b]0;PWNED\x07denied"}).encode()
reason = guess_status_code_reason(401, body)   # contains a raw ESC byte
# rendered through Rich as main does today:
#   escape()          -> CSI-2J reaches the terminal: True,  OSC-title: True
#   sanitize_markup() -> CSI-2J: False, OSC-title: False

Trigger: comfy model download --url <host> where that host answers 401 with a JSON body whose message carries escape bytes. A CivitAI-shaped mirror or a MITM'd model host is the realistic case.

What changed

Two layers, deliberately:

1. The boundary — guess_status_code_reason() (file_utils.py). The one branch that echoes server text now runs it through sanitize_value() before interpolating. Fixing it here means every consumer of the reason string benefits — the comfy model download error line, the persisted background-download state file, and comfy node install's ui.display_error_message() — rather than each print site having to remember. Markup escaping is deliberately not applied here: display_error_message renders with markup=False, so escaping backslashes would be visible on screen there.

2. The print sites — models.py:401 and :704. escape(str(x)) becomes sanitize_markup(x). sanitize_markup is sanitize_value + rich.markup.escape, so it is a strict superset of the old behavior: it keeps the existing MarkupError protection, adds ANSI/control-byte stripping, and does the str() coercion the old str(e) calls did (so those are dropped). Both layers earn their keep — the print sites still cover reasons this change does not sanitize at source, such as the message aria2 relays into a DownloadException, and the download-status / downloads row error, which is text read back from a state file a detached worker wrote.

sanitize_markup is a no-op on markup-free, escape-free text, so realistic messages render byte-identically. There is an explicit test for that.

Tests

Added to tests/comfy_cli/command/test_pretty_print_sanitize.py, reusing that file's pretty fixture (force-terminal renderer) and assert_inert() helper (no OSC 8 / OSC 0 / CSI 2J survives):

  • test_guess_status_code_reason_401_strips_server_escapes — the boundary itself.
  • test_guess_status_code_reason_leaves_benign_text_alone — the no-op guarantee on a realistic message.
  • test_download_error_is_inert / ..._with_unbalanced_markup_does_not_crashmodels.download() against a stubbed httpx.stream returning 401 with a hostile body. Nothing is faked between the response and the print site: the real guess_status_code_reason builds the reason, the real DownloadException carries it, the real except clause renders it.
  • test_download_status_row_error_is_inert / ..._with_unbalanced_markup_does_not_crash — the per-row error line under the download-status / downloads table.

Verified failing before the fix: with the two source files reverted, the three ANSI assertions fail (AssertionError: escape byte survived: 'abc123: \x1b[2J\x1b]0;PWNED...'). The two unbalanced_markup tests pass both before and after by design — they pin the MarkupError protection escape() already provided, proving this change does not regress it.

Full suite: 3045 passed, 37 skipped. ruff check / ruff format --check clean on the touched files (the repo has 16 pre-existing ruff check findings and one unformatted file on the base branch under local ruff 0.12.7; this change adds none).

Judgment calls / not done

  • Sanitizing at the file_utils boundary was the ticket's "also worth a look" suggestion, and I took it. It is the reason comfy node install is fixed too without touching custom_nodes/command.py. It is a behavior change to a shared function, so: it only ever removes C0/C1 control characters and ANSI sequences from an attacker-chosen substring, non-str values get the same str() coercion the f-string did, and the benign-text no-op is pinned by a test.
  • Adjacent, deliberately out of scope — server-supplied CivitAI metadata is still rendered unsanitized through markup. request_civitai_model_version_api() returns download_url, model_name, and baseModel straight from the API response, and those reach markup-interpreting prints a few lines above the site fixed here (Start downloading URL: {url} into {local_filepath}, and File already exists: {local_filepath}). It is a real hazard of the same class, but a different one from this ticket's (error text), and it requires compromising the pinned civitai.com host rather than an arbitrary --url host, so it is materially weaker. Flagging rather than folding it in — happy to file it as a follow-up.
  • file_utils.py's retry line (print(f"Download error (attempt ...)")) uses the builtin print, not the Rich one, and its text comes from _friendly_network_error(), which returns canned strings for HTTP statuses. No server-chosen text reaches a markup sink there, so it is untouched.

mattmillerai and others added 7 commits July 27, 2026 17:33
…derer boundary (BE-4794)

Server-supplied strings reach the terminal verbatim in pretty mode. A remote or
shared ComfyUI (comfy run --host/--port) picks the prompt_id and writes the
exception text the CLI interpolates into error output, and Rich does not save
us: rich.text.Text drops a few C0 characters but passes ESC straight through,
so a CSI/OSC sequence can clear the screen, rewrite the window title, or
repaint earlier lines to spoof CLI output.

Add comfy_cli/output/sanitize.py (C0/C1 control characters plus CSI/OSC/DCS
escape sequences, keeping tab and newline) and apply it once at the pretty
rendering boundary: error_panel's code/message/hint/details, and Renderer's
info/warn/success. The JSON and NDJSON envelope paths are untouched --
json.dumps already escapes ESC, and stripping there would mutate the data
agents parse.
…ites (BE-4794)

Stripping the escape *bytes* was only half the boundary: Rich manufactures new
ones from markup it finds in the string, so the sanitizer was bypassable.

- Add `sanitize_markup` (sanitize_value + `rich.markup.escape`) and use it
  wherever text reaches a markup-parsing sink: `Renderer.info/warn/success`
  (+ their hints) and the `details` rows `error_panel` hands to
  `Table.add_row`. Verified: a server-supplied
  `[link=https://attacker.example]x[/link]` rendered as a live OSC 8 hyperlink
  (`\x1b]8;...`), and an unbalanced `[/]` raised `rich.errors.MarkupError`,
  crashing the CLI while it was merely printing an info line.
  Values passed to `rich.text.Text` (code/message/hint) keep plain `sanitize_*`
  — `Text` never parses markup, so escaping there would show the backslashes.
- Drop the now-redundant `rich.markup.escape` in `outdated.execute`:
  double-escaping renders a visible stray backslash. Escaping at the boundary
  rather than per call site is the point of having a boundary; callers that
  genuinely want markup still have `renderer.print()`, the documented
  passthrough. No call site passed markup to these helpers.
- `_ESCAPE_SEQUENCE_RE`: add SOS (`\x98`) to the 8-bit C1 string-introducer
  class, so `\x98payload\x9c` is consumed as a unit instead of leaving
  `payload` as visible garbage — matching the 7-bit `\x1bX` form.
- `error_panel` code/message use `sanitize_value`, and `sanitize_optional`
  coerces non-strings, so a caller ignoring the `str` annotation gets `str()`
  coercion rather than a `TypeError` from `re.sub`.

Tests: 15 new cases, all verified to fail without the source fix. Realistic
messages (`list[0]`, `[2/5] downloading`, Windows paths, URLs) render
byte-identically. Full suite green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These assert on an un-wrapped substring, and `rich.print` gives no way to pass
`width` through, so the result otherwise depends on the runner's default
console width. The error-panel case fit in 80 columns with only ~10 to spare.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(BE-4794)

The renderer boundary only covers text routed through `Renderer`. Command
modules print under their own `is_pretty()` gates, and those strings never
reach it — so the headline attack from the ticket (`comfy run --host/--port`
against a hostile server) still cleared the screen and set the window title,
as @bigcat88 proved on this branch.

Two defects at each of those sites, both pre-existing on `main`:
raw ANSI in the payload reaches the terminal, and Rich manufactures new
escapes from markup it finds — with an unbalanced `[/]` raising
`MarkupError` and hard-crashing the CLI from a remote string.

Route the server-supplied values through the PR's own `sanitize_markup`
across the `comfy run` and `comfy models` surfaces: the HTTP error body and
`execution_error` payload, the submit `URLError` reason, `--verbose` node
logs, preflight validation warnings, cloud/local output paths and warnings,
the watcher's output URLs, and the asset-catalog rows (`Table.add_row`
parses markup in `str` cells, same sink as the error panel's `_kv_table`).

The two `json.dumps` sites are *not* already safe as the review suggested:
`json.dumps` escapes `\x1b` but not `[`, so a node-error message containing
`[/red]` still crashed the CLI. Verified, and they are sanitized too — the
rendered blob is byte-identical to the original JSON.

Also reword the module docstring, which claimed to be "the single sanitizing
boundary": automatic coverage stops at `Renderer`, and a bare `rich.print`
of remote text must sanitize explicitly. Saying otherwise is the "false
sense of coverage" the PR body itself warns about.

15 regression tests drive the real call sites in pretty mode with a
force-terminal console; each was verified to fail without its fix.
…BE-4794)

Self-review turned up a third surface reachable by the identical attack.
`comfy nodes ls/show/search/types/categories/path` take the same
`--host/--port` pair as `comfy run` and render `object_info` straight from
that host into markup sinks: node ids, categories, descriptions, display
names, output/input types.

Verified against a hostile `object_info`: `comfy nodes show --host <evil>`
cleared the screen and set the window title, and a description of
`boom [/] x` raised an unhandled `MarkupError`.

Leaving this open would have closed BE-4794 with the same threat still
working one command over — the exact objection raised on the run path.

The intentional markup stays intentional: the `[dim]—[/dim]` empty-outputs
fallback and the `[bold]`/`[cyan]` wrappers are ours, so only the joined
server values inside them are escaped. Search/description cells truncate
before escaping so the escapes stay balanced.

5 more regression tests drive the real commands against a hostile
`object_info`; each verified to fail without the fix.
Textual conflict in `command/run/__init__.py` only. Main refactored the
local `--wait` path: `jobs_state.write` became `_write_state`, and the
pretty "Outputs:" block moved out of the `try` to after the `finally`,
iterating `completed_payload["outputs"]` instead of `execution.outputs`.

Took main's structure wholesale and re-applied the sanitize at the block's
new home, so both intents survive: main's state-file semantics (report a
`state_file` only when the terminal write landed) and this branch's
escaping of server-chosen output filenames. Same for the `ws_disconnected`
path — main's new `server_died` state-file block kept, with
`sanitize_markup(e)` retained on the pretty line.

Merged-tree verification: 3039 passed, 37 skipped; ruff clean on the
touched files.
…h markup

`comfy model download` rendered download failures with `rich.markup.escape()`,
which neutralizes the Rich-markup half of the hazard (its inline comment said
so — it was added to stop `MarkupError`) but leaves raw ANSI/control bytes
intact. Server-chosen text therefore still reached the terminal live and could
clear the screen, rewrite the window title, or repaint earlier lines to spoof
CLI output.

The remote text arrives via `guess_status_code_reason()`. Every branch but 401
returns a canned per-status string; the 401 branch interpolates the server's own
JSON `message`, which flows into `DownloadException` and out through the two
print sites.

Fixed at both layers:

- `guess_status_code_reason` sanitizes the server-supplied `message` at the
  boundary, so every consumer of the reason string benefits (the download error
  line, the persisted background-download state, `comfy node install`) instead
  of each print site having to remember. Markup escaping is deliberately not
  applied there — not every consumer renders through a markup-interpreting sink.
- The two `comfy model download` print sites use `sanitize_markup()` instead of
  `escape()`. It is a strict superset: same `MarkupError` protection, plus
  ANSI/control-byte stripping, plus the `str()` coercion the old `str(e)` calls
  did. It still covers reasons this change does not sanitize at source, such as
  the message aria2 relays.

Regression tests drive a hostile 401 body through the real chain — real
`guess_status_code_reason`, real `DownloadException`, real print sites — and
assert nothing executable survives. Verified failing before the fix.
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b70cc1bf-93de-46c5-aa4c-12f444ab09f1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@mattmillerai
mattmillerai marked this pull request as ready for review July 29, 2026 07:57
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jul 29, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

⚠️ Review failed

Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've verified all the findings against the actual code. Key confirmations:

- `sanitize_value`/`sanitize_markup` both coerce non-str via `str()` (sanitize.py:82) — so the three Gemini findings claiming a `TypeError` from passing `e`/`row['error']`/`msg_json["message"]` without `str()` are **false positives** (dropped).
- `guess_status_code_reason` is called at file_utils.py:335 *outside* any try; foreground `download()` (models.py:398) only catches `DownloadException` → non-dict `msg_json` and i

Re-trigger by removing and re-adding the cursor-review label.

Base automatically changed from matt/be-4794-sanitize-ansi to main July 29, 2026 09:50
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 29, 2026
@bigcat88

Copy link
Copy Markdown
Contributor

This PR currently conflicts with main — GitHub reports mergeable: CONFLICTING, so it needs a rebase before I can review it and I'm skipping it in the current review sweep.

Please rebase (or merge main in) and I'll pick it up on the next pass. main moved a fair bit in the last day, including #614 (ANSI sanitisation across the pretty-print call sites) and #628 (the duplicate server_died error-code fix that had main red), so a refresh may also clear unrelated CI noise on this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop bug Something isn't working cursor-review Request Cursor bot review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants